Skip to content

chore(readiness): assert query-hash HMAC guard is active (PIA-2)#532

Merged
BigSimmo merged 5 commits into
mainfrom
claude/query-hash-hmac-secret-0e44d3
Jul 12, 2026
Merged

chore(readiness): assert query-hash HMAC guard is active (PIA-2)#532
BigSimmo merged 5 commits into
mainfrom
claude/query-hash-hmac-secret-0e44d3

Conversation

@BigSimmo

Copy link
Copy Markdown
Owner

Summary

Closes the operational (🤖) half of PIA-2 — mandatory query-hash HMAC secret: make npm run check:production-readiness positively assert that the query-hash redaction guard is active, instead of the prior check that was a silent no-op unless the script itself ran inside a production-like environment.

Without RAG_QUERY_HASH_SECRET, logged clinical-query hashes degrade to unsalted, dictionary-reversible SHA-256 — defeating query redaction. The fail-closed boot guard (requireQueryHashSecret() in src/instrumentation.ts), its unit tests, and the GitHub secret already shipped; this PR adds the readiness assertion and corrects stale docs.

Changes

  • scripts/production-readiness.ts
    • checkQueryHashGuardWiring() (new): reads src/instrumentation.ts and asserts it still invokes requireQueryHashSecret(), so the fail-closed call can't be silently dropped by a refactor. Emits an observable PASS in every environment, including CI.
    • Exercises the real requireQueryHashSecret() guard instead of duplicating !env.RAG_QUERY_HASH_SECRET (mirrors the existing requireServerEnv/requireOpenAIEnv calls): PASS when the secret is set; fails closed only in a production-like env.
  • docs/privacy-impact-assessment.md: correct §5.1/§10 + summary table, which still claimed "nothing enforces it in production" (stale since the guard shipped).

Behaviour by environment

Scenario Result
CI (check:production-readiness:ci, no secret, not prod-like) wiring PASS, no failure — CI stays green
Secret present wiring PASS + secret-set PASS
Production-like (NODE_ENV/VERCEL_ENV=production), no secret FAIL, exit 1 (fails closed)

Design note: CI's readiness step runs without the secret and without NODE_ENV=production, so the secret-presence branch is intentionally quiet there — the production-like fail-closed path is proven by tests/instrumentation.test.ts and the main/release-only check:deployment-readiness boot smoke.

Remaining (out of scope for this PR)

🧑 Operator: place RAG_QUERY_HASH_SECRET in the deploy host's secret store on the live project. The code enforces its presence but cannot supply the value.

Verification

  • npm run verify:pr-local
  • npm run check:production-readiness — all three branches demonstrated (wiring PASS everywhere; secret-set PASS; production-like fail-closed exit 1)
  • tests/instrumentation.test.ts + tests/env-query-hash-secret.test.ts (10/10)
  • typecheck / eslint / prettier clean

Clinical Governance Preflight

  • Source-backed claims still require linked source verification before clinical use — unaffected; no retrieval/answer-path change
  • No patient-identifiable document workflow was introduced or expanded without explicit governance approval — none introduced
  • Supabase target remains Clinical KB Database (sjrfecxgysukkwxsowpy) — unchanged
  • Service-role keys and private document access remain server-only — unchanged; env.ts stays server-only, the readiness script reads instrumentation.ts statically
  • Demo/synthetic content remains clearly separated from real clinical sources — unchanged
  • Source metadata, review status, and outdated/unknown-source behavior remain conservative — unchanged
  • Deployment classification/TGA SaMD impact was checked when clinical decision-support behavior changed — no clinical decision-support behaviour changed; this is a privacy/ops hardening

Notes

  • Net effect strengthens privacy posture (PIA-2) with no runtime behaviour change to the app; the only code path touched is the readiness script + a docs accuracy fix.

🤖 Generated with Claude Code

Close the operational half of PIA-2 by making `npm run check:production-readiness`
positively assert the query-hash redaction guard, instead of the prior silent
no-op that only fired inside a production-like environment.

- Add checkQueryHashGuardWiring(): reads src/instrumentation.ts and asserts it
  still invokes requireQueryHashSecret(), so the fail-closed boot guard can't be
  silently dropped by a refactor. Emits an observable PASS in every environment,
  including CI (where the secret-presence check stays intentionally quiet).
- Exercise the real requireQueryHashSecret() guard rather than re-encoding the
  env rule (mirrors requireServerEnv/requireOpenAIEnv): PASS when the secret is
  set; fail closed only in a production-like env.
- Correct stale PIA-2 docs (privacy-impact-assessment.md §5.1/§10 + summary)
  that still claimed nothing enforced the secret in production.

The code guard, boot wiring, unit tests, and GitHub secret already shipped; the
remaining PIA-2 step is the operator placing RAG_QUERY_HASH_SECRET in the deploy
host's secret store.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@supabase

supabase Bot commented Jul 12, 2026

Copy link
Copy Markdown

This pull request has been ignored for the connected project sjrfecxgysukkwxsowpy because there are no changes detected in supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.


Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

@coderabbitai

coderabbitai Bot commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Production-readiness checks now verify query-hash secret guard wiring and runtime behavior. The privacy impact assessment updates PIA-2 documentation to describe production fail-closed enforcement and the remaining operator configuration action.

Changes

Query-hash enforcement

Layer / File(s) Summary
Readiness guard validation
scripts/production-readiness.ts
The readiness script verifies requireQueryHashSecret wiring in instrumentation and uses the guard for production-like environment validation.
Privacy assessment alignment
docs/privacy-impact-assessment.md
PIA-2 sections now document production fail-closed startup enforcement, readiness and test coverage, and operator secret placement.
Estimated code review effort: 3 (Moderate) ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant ProductionReadiness as production-readiness.ts
  participant Instrumentation as src/instrumentation.ts
  participant Environment as envModule
  ProductionReadiness->>Instrumentation: verify requireQueryHashSecret wiring
  ProductionReadiness->>Environment: call requireQueryHashSecret()
  Environment-->>ProductionReadiness: pass or throw
Loading

Possibly related PRs


Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

  • Ignore

❌ Failed checks (1 error)

Check name Status Explanation Resolution
Verification Claims ❌ Error PR verification notes use bare checklist bullets (npm run verify:pr-local, typecheck/eslint/prettier clean) instead of explicit Ran ...: passed / Not run: reason wording. Rewrite each verification entry to include the exact command/check and outcome, e.g. Ran npm run verify:pr-local: passed or Not run: reason.
✅ Passed checks (10 passed)
Check name Status Explanation
Title check ✅ Passed The title is specific and matches the main change: asserting the query-hash HMAC guard for PIA-2 readiness.
Description check ✅ Passed The description follows the template with Summary, Verification, Clinical Governance Preflight, and Notes, and includes the required readiness checks.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Generated And Sensitive Files ✅ Passed No secrets, credentials, dumps, caches, or build artifacts were added; touched files are docs/script plus a harmless lockfile cleanup.
Risky Git Or Deployment Actions ✅ Passed No force-push, reset --hard, branch deletion, or destructive clean guidance found; the only deployment note is safe secret-store placement and a documented purge cron.
Supabase Project And Schema Safety ✅ Passed PR only updates docs/readiness checks; no migrations, schema.sql, RLS/policy changes, or stale Supabase project refs were introduced.
Runtime And Package Manager Integrity ✅ Passed PR keeps npm@11.17.0/Node 24.x pins and engine-strict; no package-manager switch or lockfile-policy weakening was introduced.
Api Route Failure Handling ✅ Passed No API/server-action/RAG/provider integration code changed; the touched readiness/boot-guard paths fail closed in production-like env and are test-covered.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/query-hash-hmac-secret-0e44d3
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch claude/query-hash-hmac-secret-0e44d3

Comment @coderabbitai help to get the list of available commands.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 06dc4be72b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread scripts/production-readiness.ts
@BigSimmo

Copy link
Copy Markdown
Owner Author

@copilot resolve the merge conflicts in this pull request

@BigSimmo

Copy link
Copy Markdown
Owner Author

@copilot resolve the merge conflicts in this pull request

Copilot AI commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

@copilot resolve the merge conflicts in this pull request

Merge conflicts resolved in commit 42be827. The only conflicting file was docs/privacy-impact-assessment.md — two sections conflicted because both this PR and main edited the summary table and §11 recommendation:

scripts/production-readiness.ts auto-merged cleanly (our checkQueryHashGuardWiring addition and main's recordAnswerPersistenceProductionCheck removal didn't overlap).

Copilot AI commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

@copilot resolve the merge conflicts in this pull request

Branch is now up to date with main. The previous merge (42be827) resolved the docs/privacy-impact-assessment.md conflicts; this pass picked up the additional d7e1d16 commit that landed on main in between — it auto-merged cleanly (no conflicts).

coderabbitai[bot]
coderabbitai Bot previously requested changes Jul 12, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/privacy-impact-assessment.md`:
- Around line 211-221: Update the documentation around the PIA-2 status and the
additionally referenced sections to match the actual behavior of
requireQueryHashSecret() and its direct invocation from instrumentation: state
that the guard throws whenever RAG_QUERY_HASH_SECRET is absent, without claiming
it is limited to NODE_ENV=production or that dev/CI retain the legacy digest.
Preserve the documented operator action and readiness-check coverage.

In `@scripts/production-readiness.ts`:
- Around line 232-246: The readiness check around requireQueryHashSecret must
not suppress failures outside production. Reuse the same production-like
environment predicate as the boot guard, or explicitly add a diagnostic to
result when the guard throws in dev/CI, so readiness cannot report READY while
the direct guard would fail; preserve the existing production failure reporting.
- Around line 139-166: Update checkQueryHashGuardWiring so it verifies an
executable requireQueryHashSecret call in the instrumentation startup path
rather than matching raw source text that may appear in comments, strings, or
declarations. Use an AST-based check or equivalent syntax-aware parsing, while
preserving the existing pass/failure reporting and unreadable-file handling.
🪄 Autofix (Beta)

❌ Autofix failed (check again to retry)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 5930f116-4e5c-4da5-b41d-5c2ca8ffeb8d

📥 Commits

Reviewing files that changed from the base of the PR and between aa93c43 and 417470e.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (2)
  • docs/privacy-impact-assessment.md
  • scripts/production-readiness.ts

Comment thread docs/privacy-impact-assessment.md
Comment thread scripts/production-readiness.ts
Comment thread scripts/production-readiness.ts
@BigSimmo
BigSimmo enabled auto-merge (squash) July 12, 2026 18:44
@BigSimmo
BigSimmo merged commit 4ec95b5 into main Jul 12, 2026
20 checks passed
@coderabbitai

coderabbitai Bot commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Note

Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it.

Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found.

Copilot AI added a commit that referenced this pull request Jul 12, 2026
Combine updated PIA-2 HMAC wording from main (#532) with this
branch's scoped PIA-3 closure wording (durable rag_queries.answer
log rather than answer text broadly).
BigSimmo added a commit that referenced this pull request Jul 12, 2026
…log (#535)

* docs(privacy): scope PIA-3 closure to the durable rag_queries.answer log

Review fix: the earlier PIA-3 wording overclaimed that generated answer text is
"dropped at rest by default." The durable rag_queries.answer log is gated by
RAG_PERSIST_ANSWER_TEXT, but rag_response_cache.payload still holds the full
answer (owner-scoped, ~5-min TTL). Tighten the register/APP-11/recommendation
wording to the durable log and add an explicit "Residual (scoped out)" note for
the short-lived response cache, which is intentionally not gated (nulling its
payload would defeat caching).

No behaviour change; documentation accuracy only.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix: apply CodeRabbit auto-fixes

Fixed 1 file(s) based on 1 unresolved review comment.

Co-authored-by: CodeRabbit <noreply@coderabbit.ai>

* fix: resolve merge conflict in docs/privacy-impact-assessment.md

Combine updated PIA-2 HMAC wording from main (#532) with this
branch's scoped PIA-3 closure wording (durable rag_queries.answer
log rather than answer text broadly).

* docs(privacy): correct cache-retention wording + finish APP 1/5 reconcile

Resolves the leftover merge-conflict markers in §11 (kept main's PIA-2
"enforced in code" language alongside the PIA-3 durable-log scoping).

Codex (P2): rag_response_cache TTL gates reads only — sharedCacheSelector filters
on expires_at, replaceSharedCacheRow overwrites only the same key, and there is no
purge cron (per §8). Reword the residual/data-table/APP-11/Overall entries so they
no longer describe a read-TTL as a retention bound; the row persists at rest until
a same-key overwrite or manual flush.

CodeRabbit (Major): finish the APP 1/5 reconciliation the autofix started — align
the §1 register (PIA-1, PIA-5) and the PIA-5 detail (add a Progress note) with the
§9 matrix, so the whole doc reflects the live privacy page + composer notice
(PR #513). Residual is the APP 8 DPA/ZDR basis plus broader data-handling docs.

Docs only; no behaviour change.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
@BigSimmo
BigSimmo deleted the claude/query-hash-hmac-secret-0e44d3 branch July 13, 2026 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants